Make select_algorithm more agnostic about being in the object or type domain #32
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
lkdvos
left a comment
lkdvos
left a comment
There was a problem hiding this comment.
I'm definitely happy to delay going to the type domain as much as possible, but then I would probably try and be meticulous about that and make sure that default_algorithm and default_f_algorithm have fallback implementations that go to the type domain, if that is possible without ambiguities etc?
I left some comments that apply to each of the implementations and didn't copy them over since we can discuss first.
src/interface/eigh.jl
Outdated
| function default_algorithm(::typeof($f), ::Type{A}; kwargs...) where {A} | ||
| return default_eigh_algorithm(A; kwargs...) | ||
| end |
There was a problem hiding this comment.
Is this to avoid ambiguities somewhere, or to improve type stability?
In principle this should already be caught by the definition above, so if it is for type stability I would guess it should be possible to merge them, possibly with an @inline annotation if necessary
There was a problem hiding this comment.
I added it to remove an ambiguity error raised by Aqua, but I don't think both definitions are needed, I'll try removing one of them.
src/interface/eigh.jl
Outdated
| alg_eigh = select_algorithm(eigh_full!, A, alg; kwargs...) | ||
| return TruncatedAlgorithm(alg_eigh, select_truncation(trunc)) | ||
| end | ||
| function select_algorithm(::typeof(eigh_trunc), A, alg; trunc=nothing, kwargs...) |
There was a problem hiding this comment.
Same comment about moving this to @algdef
|
@lkdvos I've addressed all of your comments, it is definitely simpler now, thanks for the feedback. |
|
Ah that's too bad, I thought I double checked but I guess there are some ambiguity issues. |
Fixed by the latest commit, it was a simple fix. @lkdvos this is ready for another review. |
src/algorithms.jl
Outdated
| @inline function default_algorithm(::typeof($f), A; kwargs...) | ||
| return default_algorithm($f!, A; kwargs...) | ||
| end | ||
| # fix ambiguity error |
There was a problem hiding this comment.
Out of curiosity, do you think it would also be possible to not have this ambiguity by not specializing on types in the different implementations? I do have to say that I'm losing track a bit of where everything is going, when it is going to the type domain etc, so I might be mistaken here
There was a problem hiding this comment.
I think the root of this ambiguity are these definitions:
MatrixAlgebraKit.jl/src/algorithms.jl
Lines 108 to 112 in 750c096
There was a problem hiding this comment.
I think it makes sense for the implementations for particular functions to be defined on types, since then with that generic forwarding you can either write default_algorithm(svd_compact!, randn(2, 2)) or default_algorithm(svd_compact!, Matrix{Float64}).
There was a problem hiding this comment.
Though thinking about this, maybe that means in this case we can just define:
@inline function default_algorithm(::typeof($f), ::Type{A}; kwargs...) where {A}
return default_algorithm($f!, A; kwargs...)
endI'll try that out.
There was a problem hiding this comment.
Ok, that works. However, the issue with that design is that for default_algorithm definitions that are defined on instances and not types, this forwarding from out-of-place to in-place definitions doesn't happen automatically. So, it seems better to have both definitions.
|
@mtfishman , from what I can see from the latest review I gave, the only question is about avoiding ambiguities. As this tends to be a do it until it works kind of thing, I'm okay with you merging this and if really necessary fixing things afterwards |
Sounds good. I think the current approach for that is reasonable (I share your skepticism but also looking over it again I couldn't think of anything better to do) but I'd be happy to reassess as we go along. |
|
I'll review this PR in an hour or so. |
|
Ok, this is ready for another review. |
|
I don't know why it is showing that some of the tests aren't running, I'll close and reopen to see if that fixes it. |
Jutho
left a comment
Jutho
left a comment
There was a problem hiding this comment.
I left one final question about the eig test, but otherwise this looks good to me.
|
Not sure why the "6 pending checks" remain in place, even though they have also successfully ran. |
I'm confused by that too. The tests are passing and I don't think anything changed with the Github Actions configuration recently, maybe it is just a bug in the Github interface. I tried closing and reopening the PR but that didn't fix it. Otherwise, this is good to go from my end once tests pass again, it will need another approval before merging. |
|
For future reference: the tests were being very annoying, somehow it seems like github started naming them differently, which conflicts with the rulesets (that have to be name-based for some reason). I've now altered the rulesets to take the new named tests, which is stranged since both appear in the options of the rulesets with the same name, so I basically had to trial-and-error until I had the correct ones. Anyways, hopefully this is now fixed for future PRs, this is good to merge for me. |
|
Thanks for the fix @lkdvos, I'll merge and register. |
Followup to #30.
While trying out #30, I realized the logic here is slightly breaking: https://github.com/QuantumKitHub/MatrixAlgebraKit.jl/blob/v0.2.1/src/algorithms.jl#L79-L100. The issue is that
select_algorithmeagerly changes the input into the type domain, so then existing overloads ofdefault_algorithm/default_f_algorithmthat are defined on instances get ignored. This PR makes that code a bit more generic so that types or instances get passed through todefault_algorithm.